Skip to content
This repository was archived by the owner on Sep 15, 2025. It is now read-only.

Support tracking HTTP request overall progress and cancellation - #669

Merged
crazytonyli merged 3 commits into
trunkfrom
url-session-helpers-overall-progress
Jan 11, 2024
Merged

Support tracking HTTP request overall progress and cancellation#669
crazytonyli merged 3 commits into
trunkfrom
url-session-helpers-overall-progress

Conversation

@crazytonyli

Copy link
Copy Markdown
Contributor

Description

Note

This PR is built on top of #668

There are many existing APIs that returns a Progress instance, which can be used to track progress and some can be used to perform cancellation. This PR implements these features in the new URLSession helper.

The existing APIs return a Progress as function return value. We can't do the same, because the new helper is an async function: the Progress will be no use if it's used as a return value. I used a parent progress instance instead. The caller now needs to pass in a valid Progress instance that's created exclusively for the HTTP request, and it'll be completed upon the completion of the HTTP request.

Another approach I tried is using a inout Progress argument, so that we don't need to create a duplicated progress instance. I envisioned it can be used as below, but ended up getting a compiler error.

var progress = Progress(totalUnitCount: 100)
Task {
    // Compiler error. The `progress` reference can't be captured in this closure.
    let _ = await perform(request, &progress)
}
// Use the progress to track the HTTP request.
track(progress)

Testing Details

See the added unit tests.


  • Please check here if your pull request includes additional test coverage.
  • I have considered if this change warrants release notes and have added them to the appropriate section in the CHANGELOG.md if necessary.

Base automatically changed from url-session-helpers-error-on-status-code to trunk January 11, 2024 02:48
/// parameter, which must satisify following requirements:
/// - `totalUnitCount` must not be zero.
/// - `completedUnitCount` must be zero.
/// - It's used exclusivity for tracking the HTTP request overal progress: No children in its progress tree.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// - It's used exclusivity for tracking the HTTP request overal progress: No children in its progress tree.
/// - It's used exclusively for tracking the HTTP request overall progress: No children in its progress tree.

Comment on lines +16 to +21
/// You can track the HTTP request's overall progress by passing a `Progress` instance to the `fulfillingProgress`
/// parameter, which must satisify following requirements:
/// - `totalUnitCount` must not be zero.
/// - `completedUnitCount` must be zero.
/// - It's used exclusivity for tracking the HTTP request overal progress: No children in its progress tree.
/// - `cancellationHandler` must be nil. You can call `fulfillingProgress.cancel()` to cancel the ongoing HTTP request.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about encoding these constraints in either a dedicated type that wraps Progress or a subclass/pre-configured instance?

Type wrapping Progress

More coding and less flexibility for advanced users, but no chance of misuse since we'd be controlling everything from here

Subclass / Preconfigured instance

Could be misuses, but by letting the method accept a Progress type, we'd give power users more flexibility

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: I saw the assert below. They do a good job at ensuring runtime safety. Still, from a "how many things does a user need to know?" perspective, I think leveraging the type system somehow would be beneficial.


let (body, response) = result
if let parentProgress, parentProgress.totalUnitCount > parentProgress.completedUnitCount {
let pending = parentProgress.totalUnitCount - parentProgress.completedUnitCount

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this math given the assert above for completedUnitCount == 0?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I don't think it's needed. But, in my head, the code expresses the idea more explicitly: letting the task.progress fill parentProgress without overflowing.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair 👍

@crazytonyli
crazytonyli marked this pull request as ready for review January 11, 2024 22:06
@crazytonyli
crazytonyli merged commit 19bdfec into trunk Jan 11, 2024
@crazytonyli
crazytonyli deleted the url-session-helpers-overall-progress branch January 11, 2024 22:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants